Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

promises: use encoding in readFile #19296

Closed
wants to merge 2 commits into from
Closed

promises: use encoding in readFile #19296

wants to merge 2 commits into from

Conversation

benjamingr
Copy link
Member

Fixes: #19286

@nodejs/fs

@benjamingr benjamingr added fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. labels Mar 12, 2018
@benjamingr benjamingr changed the title Promises: use encoding in readFile promises: use encoding in readFile Mar 12, 2018
@benjamingr
Copy link
Member Author

Random question - do I label this fs or promises in the commit message title?

thefourtheye
thefourtheye approved these changes Mar 12, 2018
View reviewed changes
test/parallel/test-fs-promises-writefile.js Outdated
async function doReadWithEncoding() {
const data = await fsPromises.readFile(dest, 'utf-8');
const buf = fs.readFileSync(dest, 'utf-8');
assert.deepStrictEqual(buf, data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert the type of data and buf as well, here?

Copy link
Member Author

@benjamingr benjamingr Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, good idea

promises: use encoding in readfile
6469a62 
Use the encoding parameter passed to fsPromises.readFile if it is
passed. Currently the encoding parameter is ignored in fsPromises

PR-URL: nodejs#19296
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Fixes: nodejs#19286
@devsnek
Copy link
Member

devsnek commented Mar 12, 2018

fs/promises? I think we use commas to separate subsystems so slash should be safe

@benjamingr
Copy link
Member Author

CI looks green sans an unrelated http2 failure: https://ci.nodejs.org/job/node-test-commit/16860//

@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2018
jasnell
jasnell reviewed Mar 12, 2018
View reviewed changes
lib/fs/promises.js Outdated
@@ -157,7 +157,12 @@ async function readFileHandle(filehandle, options) {
chunks.push(buffer.slice(0, totalRead));
} while (totalRead === chunkSize);

return Buffer.concat(chunks);
var buffer = Buffer.concat(chunks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/var/const

@joyeecheung
Copy link
Member

joyeecheung commented Mar 12, 2018 •
edited
Loading

I think fs subsystem should be enough, if I am not mistaken promises is for the implementation of promise itself

@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/13648/

@lpinca
Copy link
Member

lpinca commented Mar 24, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/13849/

@lpinca
Copy link
Member

lpinca commented Mar 24, 2018

Landed in e06ad5f.

lpinca pushed a commit that referenced this pull request Mar 24, 2018
@lpinca
fs: use encoding in readFile
e06ad5f 
Use the encoding parameter passed to fsPromises.readFile if it is
passed. Currently the encoding parameter is ignored in fsPromises.

PR-URL: #19296
Fixes: #19286
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
@lpinca lpinca closed this Mar 24, 2018
lpinca
lpinca reviewed Mar 24, 2018
View reviewed changes
test/parallel/test-fs-promises-writefile.js
const data = await fsPromises.readFile(dest, 'utf-8');
const syncData = fs.readFileSync(dest, 'utf-8');
assert.strictEqual(typeof data, 'string');
assert.deepStrictEqual(data, syncData);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.strictEqual() was probably better here, but I've already landed this and it's not a biggie so I will not force push.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca why?

Copy link
Member

@lpinca lpinca Mar 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a string no? No need to be deep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.